Skip to content

[PLT-0] LBOX Namespace, Plugin Style SDK #1656

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

adrian-chang
Copy link
Contributor

@adrian-chang adrian-chang commented Jun 5, 2024

Description

https://labelbox.atlassian.net/wiki/spaces/PLT/pages/2658074642/TDD+Labelbox+Python+Repo+Organization

Labelbox lbox example + initial develop.yaml additions

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Document change (fix typo or modifying any markdown files, code comments or anything in the examples folder only)

All Submissions

  • Have you followed the guidelines in our Contributing document?
  • Have you provided a description?
  • Are your changes properly formatted?

New Feature Submissions

  • Does your submission pass tests?
  • Have you added thorough tests for your new feature?
  • Have you commented your code, particularly in hard-to-understand areas?
  • Have you added a Docstring?

Changes to Core Features

  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you updated any code comments, as applicable?

@adrian-chang adrian-chang changed the title Achang/plt 0 lbox example [PLT-0] LBOX Namespace, Plugin Style SDK Jun 6, 2024
@adrian-chang adrian-chang force-pushed the achang/plt-0-lbox-example branch 2 times, most recently from f24fc7c to b12032b Compare June 6, 2024 09:00
@adrian-chang adrian-chang force-pushed the achang/plt-0-lbox-example branch from c277945 to c66fbb6 Compare July 5, 2024 06:07
@adrian-chang adrian-chang temporarily deployed to Test-PyPI-lbox-example July 5, 2024 06:13 — with GitHub Actions Inactive
@adrian-chang adrian-chang temporarily deployed to Test-PyPI-lbox-example July 5, 2024 06:28 — with GitHub Actions Inactive
@adrian-chang adrian-chang temporarily deployed to Test-PyPI-lbox-example July 5, 2024 06:45 — with GitHub Actions Inactive
@adrian-chang adrian-chang temporarily deployed to Test-PyPI-lbox-example July 5, 2024 07:00 — with GitHub Actions Inactive
@adrian-chang adrian-chang force-pushed the achang/plt-0-lbox-example branch from e62ef86 to d938d4b Compare July 9, 2024 03:01
@adrian-chang adrian-chang marked this pull request as ready for review July 9, 2024 03:01
@adrian-chang adrian-chang requested a review from a team as a code owner July 9, 2024 03:01
@adrian-chang adrian-chang temporarily deployed to Test-PyPI-lbox-example July 9, 2024 03:04 — with GitHub Actions Inactive
@vbrodsky
Copy link
Contributor

vbrodsky commented Jul 9, 2024

I've reviewed the wonderful wiki documentation [Wiki Link] and skimmed through the pull request related to the proposed changes for the SDK. Based on my understanding it seems the proposal aims to address several objectives:

  • Enable safe contributions of external packages to the SDK by external developers.
  • Facilitate workload distribution between different subteams within the SDK development process.
  • Potentially introduce a mechanism for deprecating SDK features.

Complexity Concerns: While I appreciate the potential benefits of these changes, I'm concerned that the proposed solution might lead to an increase in overall SDK development complexity in the near future. Personally, I would prefer to minimize such complexity.

Before moving forward with this specific proposal, I believe it would be valuable to take a step back and identify the most pressing challenges we need to address for the SDK's future development. By focusing on the core problems, we can potentially explore alternative solutions that achieve our goals without introducing unnecessary complexity.

I'd be happy to discuss this further and collaborate on defining a clear roadmap for the SDK's future.

@adrian-chang
Copy link
Contributor Author

adrian-chang commented Jul 9, 2024

Complexity Concerns: While I appreciate the potential benefits of these changes, I'm concerned that the proposed solution might lead to an increase in overall SDK development complexity in the near future. Personally, I would prefer to minimize such complexity.

I'd like hear this concern about complexity. I think its much simpler to operate in a bunch of micro / smaller packages (although there is acknowledged organic overhead of just doing more than a single package) than to deal with a single monolithic SDK package. You get versioning, the ability to do small breaking changes, run a small subset of tests, and the flexibility to do a lot more things than having to worry about changes to code in other areas of the codebase.

Breaking things up is always a tough thing to do, but in fact we really aren't overtly breaking this up for the sake of breaking things up. In fact, I wouldn't really even consider this breaking up the SDK, more about enforcing strong boundaries been modules where that doesn't exist at all currently.

To be 100% clear, there are no required or mandatory near future changes. SDK3 can be developed as SDK3 (this PR makes no impact on it). Any of these changes can be back-ported to SDK3 also.

Happy to discuss, but the main objective is to organically move things over in time to assist to getting an SDK4.

Before moving forward with this specific proposal, I believe it would be valuable to take a step back and identify the most pressing challenges we need to address for the SDK's future development.

If you feel there is a pressing need somewhere else, please create a proposal and execute on it and we can compare then.

Copy link
Contributor

@vbrodsky vbrodsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approvable, just had a couple of questions

- name: Integration
working-directory: libs/${{ matrix.package }}
env:
LABELBOX_TEST_API_KEY: ${{ secrets[matrix.api-key] }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are these matrix attribures (api-key, package, etc) defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See .github/actions/lbox-matrix/index.js

const startingMatrix = [
{
"python-version": "3.8",
"api-key": "STAGING_LABELBOX_API_KEY_2",
"da-test-key": "DA_GCP_LABELBOX_API_KEY"
},
{
"python-version": "3.9",
"api-key": "STAGING_LABELBOX_API_KEY_3",
"da-test-key": "DA_GCP_LABELBOX_API_KEY"
},
{
"python-version": "3.10",
"api-key": "STAGING_LABELBOX_API_KEY_4",
"da-test-key": "DA_GCP_LABELBOX_API_KEY"
},
{
"python-version": "3.11",
"api-key": "STAGING_LABELBOX_API_KEY",
"da-test-key": "DA_GCP_LABELBOX_API_KEY"
},
{
"python-version": "3.12",
"api-key": "STAGING_LABELBOX_API_KEY_5",
"da-test-key": "DA_GCP_LABELBOX_API_KEY"
}
];

@adrian-chang adrian-chang merged commit bab9746 into develop Jul 17, 2024
17 checks passed
@adrian-chang adrian-chang deleted the achang/plt-0-lbox-example branch July 17, 2024 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants